Skip to content

fix(client): GetServers() throws FormatException on DNS hostnames - #497

Open
currantw wants to merge 14 commits into
valkey-io:mainfrom
currantw:419-get-servers-endpoint-bug
Open

fix(client): GetServers() throws FormatException on DNS hostnames#497
currantw wants to merge 14 commits into
valkey-io:mainfrom
currantw:419-get-servers-endpoint-bug

Conversation

@currantw

@currantw currantw commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fix ConnectionMultiplexer.GetServers() to support DNS hostnames in cluster topology responses. Previously, it used IPEndPoint.Parse() which throws FormatException when node addresses are DNS names (e.g., AWS ElastiCache endpoints like my-cluster.abc123.cache.amazonaws.com:6379).

Issue Link

Closes #419.

Features and Behaviour Changes

  • GetServers() now correctly returns DnsEndPoint instances for cluster nodes with DNS hostnames, instead of throwing FormatException.
  • GetEndPoints(false) works for clusters with DNS-based topology.
  • No behaviour change for clusters with IP-based topology (returns IPEndPoint as before).

Implementation

Replaced IPEndPoint.Parse(addr) with Format.TryParseEndPoint(addr, ...) in the cluster branch of GetServers(). This utility already exists in the codebase and is used throughout (e.g., ConfigurationOptions parsing, EndPointCollection). It returns IPEndPoint for IP addresses and DnsEndPoint for hostnames.

Extracted the LINQ expression into an explicit loop for readability.

Limitations

⚪ None

Testing

Integration tests added: DnsGetServersTests (gated behind VALKEY_GLIDE_DNS_TESTS_ENABLED) starts a cluster with cluster-announce-hostname and verifies:

  • GetServers() returns DnsEndPoint instances
  • GetEndPoints(false) returns valid DNS endpoints that can be looked up via GetServer(endpoint)

These tests depend on valkey-io/valkey-glide#6667 which adds cluster-announce-hostname support to cluster_manager.py.

Manual verification: Tested end-to-end against a 3-node Valkey 9.0.3 cluster configured with cluster-announce-hostname valkey.glide.test.tls.com:

  • GetServers() returned 3 DnsEndPoint instances without throwing.
  • GetEndPoints(false) returned correct DNS endpoints.
  • GetServer(endpoint) successfully looked up each discovered endpoint.
  • All existing unit tests pass (558/558).

Related Issues

Checklist

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated and all checks pass.
  • CHANGELOG.md, README.md, DEVELOPER.md, and other documentation files are updated.
  • Destination branch is correct - main or release
  • Create merge commit if merging release branch into main, squash otherwise.
  • All TODOs referencing issue(s) closed by this PR have been resolved.

currantw added 2 commits July 30, 2026 11:40
@currantw
fix(client): use Format.TryParseEndPoint in GetServers() to support D…
c8c500b 
…NS hostnames

ConnectionMultiplexer.GetServers() used IPEndPoint.Parse() to parse
cluster node addresses, which throws FormatException when the cluster
topology contains DNS hostnames (e.g., AWS ElastiCache endpoints).

Replace with Format.TryParseEndPoint which correctly handles both IP
addresses (returning IPEndPoint) and DNS hostnames (returning
DnsEndPoint).

Closes valkey-io#419

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
test(integration): add DNS hostname topology tests for GetServers()
ed10d5e 
Add integration tests that start a cluster with cluster-announce-hostname
and verify GetServers()/GetEndPoints(false) correctly return DnsEndPoint
instances. Also thread a `host` parameter through ServerManager and
Server/ClusterServer to support passing --host to cluster_manager.py.

These tests are gated behind VALKEY_GLIDE_DNS_TESTS_ENABLED and require
the cluster_manager.py change from valkey-io/valkey-glide#6667.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw currantw self-assigned this Jul 30, 2026
currantw added 6 commits July 30, 2026 14:08
@currantw
refactor: move DNS tests to DnsTests.cs and use List in GetServers()
d273bf3 
- Move GetServers/GetEndPoints DNS topology tests into existing DnsTests
  class with DnsTestsFixture (add DnsClusterServer to the fixture).
- Use List<ValkeyServer> instead of array+index in GetServers().
- Add host parameter to StandaloneServer for consistency.
- Make TLS server startup non-fatal in DnsTestsFixture so non-TLS
  tests can still run when TLS servers fail to start.
- Remove separate DnsGetServersTests.cs file.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Minor GetServers cleanup
d1c9be6 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Cleanup DnsTests
2bda1c1 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
style(test): replace Assert.True(x.Count > 0) with Assert.NotEmpty(x)
04c19e4 
Use idiomatic xUnit assertion across integration tests.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
chore(deps): bump valkey-glide submodule and fix FFI breaking change
0ef0dd8 
Bump valkey-glide submodule to cf3d56f08 which includes
cluster-announce-hostname support in cluster_manager.py (#6667).

Add missing `recovery_requests_queue_size` field to ConnectionRequest
in ffi.rs to fix compilation against the updated glide-core.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
fix(test): make DnsClusterServer startup non-fatal in DnsTestsFixture
022ee99 
The DNS cluster server (using cluster-announce-hostname) may fail to
start in CI environments where wait_for_all_topology_views times out
because the CLUSTER SLOTS response reports IPv6 addresses instead of
the announced hostname. Wrap in try/catch and skip tests when null.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
currantw requested a review from Copilot July 31, 2026 02:07
Copilot AI reviewed Jul 31, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes ConnectionMultiplexer.GetServers() (and therefore GetEndPoints(false)) for cluster topologies that advertise DNS hostnames by switching endpoint parsing from IPEndPoint.Parse(...) (which throws on hostnames) to Format.TryParseEndPoint(...) (which can return DnsEndPoint).

Changes:

  • Update cluster GetServers() discovery to parse addresses into EndPoint (supporting DNS hostnames).
  • Extend test server utilities to start clusters with an announced hostname (--host).
  • Add/adjust integration tests to validate DNS-based discovery paths and tighten some assertions (Assert.NotEmpty).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/Valkey.Glide.TestUtils/ServerManager.cs Adds optional host arg to pass --host to the cluster manager script.
tests/Valkey.Glide.TestUtils/Server.cs Plumbs optional host through server wrappers (cluster/standalone).
tests/Valkey.Glide.IntegrationTests/ScriptingCommandTests.cs Replaces Count > 0 assertions with Assert.NotEmpty.
tests/Valkey.Glide.IntegrationTests/DnsTests.cs Adds integration tests for GetServers() / GetEndPoints(false) with DNS topology; extends fixture to attempt starting a DNS-announced cluster.
tests/Valkey.Glide.IntegrationTests/ClusterClientTests.cs Replaces Count > 0 assertions with Assert.NotEmpty.
sources/Valkey.Glide/Abstract/ConnectionMultiplexer.cs Uses Format.TryParseEndPoint in cluster GetServers() to support DNS hostnames.
rust/src/ffi.rs Adds new struct field initialization (recovery_requests_queue_size: None) to keep Rust-side request construction in sync.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/Valkey.Glide.IntegrationTests/DnsTests.cs Outdated
Comment thread tests/Valkey.Glide.IntegrationTests/DnsTests.cs Outdated
@currantw
fix(test): address Copilot review comments on DnsTests
deae300 
- Add GetServer(endpoint) assertion to GetEndPoints test to verify
  returned DnsEndPoints are usable for server lookup.
- Remove try/catch around DnsClusterServer and TLS server startup so
  failures are loud instead of silently skipped.
- Remove unnecessary SkipWhen null guards.

Signed-off-by: currantw <taylor.curran@improving.com>
currantw
currantw commented Jul 31, 2026
View reviewed changes
Comment thread rust/src/ffi.rs Outdated
currantw
currantw commented Jul 31, 2026
View reviewed changes
Comment thread tests/Valkey.Glide.IntegrationTests/ScriptingCommandTests.cs
@currantw currantw added this to the 1.2 milestone Jul 31, 2026
@currantw
refactor(test): overhaul DNS tests with TLS coverage and CreateClient…
f1a2cd0 
…Async(host)

- Add optional `host` parameter to `CreateClientAsync` on Server,
  ClusterServer, and StandaloneServer to support connecting via a
  custom hostname.
- Replace `BuildClient` helper in DnsTests with `GetServer` +
  `CreateClientAsync(host)`.
- Pass `host: HostnameTls` to all 4 fixture servers so DNS hostname
  is used consistently.
- Use `ClusterAndTlsMode` data member to test all cluster/TLS
  combinations for connection tests.
- Use `TlsMode` data member for GetServers/GetEndPoints tests with
  `TrustIssuer` for TLS cert validation.
- Add static constructor skip for class-level DNS test gating.

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw currantw added core Core library (`sources/Valkey.Glide/`) compatibility StackExchange.Redis API compatibility labels Jul 31, 2026
currantw added 4 commits July 31, 2026 10:52
@currantw
Merge branch 'main' of https://github.com/valkey-io/valkey-glide-csharp…
66fe501 
… into 419-get-servers-endpoint-bug
@currantw
Add explicit unpauses for flaky tests
678c82b 
Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Merge branch 'main' of https://github.com/valkey-io/valkey-glide-csharp…
53d06ab 
… into 419-get-servers-endpoint-bug

Signed-off-by: currantw <taylor.curran@improving.com>
@currantw
Merge branch 'main' of https://github.com/valkey-io/valkey-glide-csharp…
3ddd6fc 
… into 419-get-servers-endpoint-bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@xShinnRyuu xShinnRyuu xShinnRyuu approved these changes
@prateek-kumar-improving prateek-kumar-improving Awaiting requested review from prateek-kumar-improving

At least 2 approving reviews are required to merge this pull request.

Assignees

@currantw currantw

Labels

compatibility StackExchange.Redis API compatibility core Core library (`sources/Valkey.Glide/`)

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

fix(client): ConnectionMultiplexer.GetServers uses System.Net.IPEndpoint instead of System.Net.Endpoint, causing exceptions with Elasticache

3 participants

@currantw @xShinnRyuu